Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Claude finished @chitalian's task —— View job PR Review Complete ✅Todo List:
Score: 6/10 - Has a critical logic bug in token calculation that needs fixing Critical Issues1. Critical Bug: Inconsistent Total Token Calculation 🚨There's a discrepancy between the test expectations and the implementation logic that reveals a potential bug: In the streaming transformer ( total_tokens:
finalInputTokens +
finalOutputTokens +
(cachedTokens ?? 0) +
(cacheWriteTokens ?? 0),But the test expects ( expect(usageChunk.usage.total_tokens).toBe(4 + 51713 + 813);Problem: The implementation adds cache tokens to 2. Logic Inconsistency Between Streaming and Non-StreamingNon-streaming version ( total_tokens: anthropicUsage.input_tokens + anthropicUsage.output_tokens + cachedTokens + cacheWriteTokens,Both implementations:
Specific RecommendationsFix 1: Correct Token Accounting LogicCurrent logic issue in // ❌ Current - inconsistent accounting
this.finalUsage = {
prompt_tokens: finalInputTokens, // Only input_tokens (4)
completion_tokens: finalOutputTokens,
total_tokens: finalInputTokens + finalOutputTokens + (cachedTokens ?? 0) + (cacheWriteTokens ?? 0), // Includes cacheRecommended fix: // ✅ Fixed - consistent accounting
this.finalUsage = {
prompt_tokens: finalInputTokens + (cachedTokens ?? 0) + (cacheWriteTokens ?? 0), // Include cache in prompt
completion_tokens: finalOutputTokens,
total_tokens: finalInputTokens + finalOutputTokens + (cachedTokens ?? 0) + (cacheWriteTokens ?? 0),Apply the same fix to non-streaming version ( // ✅ Fixed
usage: {
prompt_tokens: anthropicUsage.input_tokens + cachedTokens + cacheWriteTokens,
completion_tokens: anthropicUsage.output_tokens,
total_tokens: anthropicUsage.input_tokens + anthropicUsage.output_tokens + cachedTokens + cacheWriteTokens,Fix 2: Update Test ExpectationsIn // ✅ Update to match corrected logic
expect(usageChunk.usage.prompt_tokens).toBe(4 + 51713); // Include cache in prompt_tokens
expect(usageChunk.usage.total_tokens).toBe(4 + 51713 + 813);Minor Issues1. Inconsistent Cache Detail MappingIn streaming ( cache_write_details: {
write_5m_tokens:
this.cacheCreationDetails?.ephemeral_5m_input_tokens ??
cacheWriteTokens ?? // ❌ Fallback to wrong value
0,Should be: write_5m_tokens:
this.cacheCreationDetails?.ephemeral_5m_input_tokens ?? 0, // ✅ Only use specific value2. Test Coverage GapThe tests don't verify the edge case where both Recommended additional test: it("should handle both cache creation and read tokens", () => {
// Test with both cache_creation_input_tokens: 1000 AND cache_read_input_tokens: 2000
// to ensure proper handling of mixed cache scenarios
});Code Quality ObservationsStrengths
Areas for Improvement
SummaryThis PR addresses an important token calculation issue for Anthropic's caching features, but has a critical bug where the token accounting is mathematically inconsistent. The Must fix before merging:
|
Ticket
Link to the ticket(s) this pull request addresses.
Component/Service
What part of Helicone does this affect?
Type of Change
Deployment Notes
Screenshots / Demos
Extra Notes
Any additional context, considerations, or notes for reviewers.
Context
Why are you making this change?
Screenshots / Demos